Conversation
adkozlov
force-pushed
the
adkozlov/ALT-10990
branch
from
a54f529 to
e331513
Compare
|
@meanmail This is a pretty important update in the deployment scripts. Also, now all the phases are executed just once and in a correct order. Tests failures don't fail the whole pipeline, it can be done later. |
meanmail
left a comment
meanmail
left a comment
There was a problem hiding this comment.
Review Summary
Several issues found — some are blockers, some are suggestions. See inline comments for details.
Blockers:
- The "Upload plugin to Marketplace" step will never execute (trigger mismatch + missing step id)
org.gradle.daemon=falseandorg.gradle.workers.max=2ingradle.propertieswill slow down local development for all contributors
Other issues:
3. Tests with continue-on-error don't block the workflow — broken artifacts get uploaded
4. Timestamp-based version format conflicts with the existing YYYY.N scheme used for Marketplace
5. Missing newline at end of both files
6. Typo: "Coretto" → "Corretto"
meanmail
left a comment
meanmail
left a comment
There was a problem hiding this comment.
Two blocking issues need to be addressed before merging:
-
Marketplace publish step is dead code — the workflow only triggers on
pull_request, but the publish condition requiresevent_name == 'push'onrefs/heads/main. Alsosteps.run-tests.outcomereferences a non-existent step id. See inline comments for fix. -
CI-specific Gradle settings leak into local development —
org.gradle.daemon=falseandorg.gradle.workers.max=2ingradle.propertiesaffect all developers. These should be passed via CLI args in the workflow instead.
See inline comments for details and suggested fixes.
|
BTW there's a part of the reason why I decided to exclude tests from the results. |
meanmail
left a comment
meanmail
left a comment
There was a problem hiding this comment.
All comments addressed. LGTM!
2 participants
No description provided.